Improve FRAME storage docs #1714
Conversation
wentelteefje
added
T1-FRAME
| /// #[pallet::storage_prefix = "OtherFoo"] | ||
| /// #[pallet::unbounded] | ||
| /// pub type Foo<T> = StorageValue<_, u32, ValueQuery>; | ||
| /// } |
There was a problem hiding this comment.
another idea is for you to add a single test/snippet here to each storage type where you re-create the storage key to demonstrate the point. Something like:
fn main() {
let expected_key = twox(pallet_name) ++ twox(Foo)
let key = Foo::hashed_key();
assert_eq!(expected., key);
}
Sadly would require you to build a runtime, but thanks to all the recent work it is only a few lines of code now :)
substrate/frame/support/src/lib.rs
Outdated
| /// #[frame_support::pallet] | ||
| /// mod pallet { | ||
| /// use frame_support::pallet_prelude::*; | ||
| /// |
There was a problem hiding this comment.
newlines can probably go away for readability?
substrate/frame/support/src/lib.rs
Outdated
| /// | ||
| /// #[pallet::call] | ||
| /// impl<T: Config> Pallet<T> { | ||
| /// fn do_stuff(_origin: OriginFor<T>, arg: u32) { |
There was a problem hiding this comment.
we can faily easily add something like this:
fn main() {
let call = pallet::Call::do_stuff { .. }
let origin = ..
call.dispatch(origin);
}
substrate/frame/support/src/lib.rs
Outdated
| /// | ||
| /// The following macros can be used in conjunction with the storage macro: | ||
| /// | ||
| /// * [`macro@getter`]: Creates a custom getter function. |
There was a problem hiding this comment.
I think these don't fit great into the story so far. Moving them toward the end might improve, but no sure. Do you think they are well explained in this position, in the overall narrative?
substrate/frame/support/src/lib.rs
Outdated
| /// ### Hashers | ||
| /// | ||
| /// For all storage types, except `StorageValue`, a set of hashers needs to be specified. The choice | ||
| /// of hashers is crucial, especially in production chains. The purpose of |
There was a problem hiding this comment.
This is one that I would probably tackle in a ref-doc, as it is too detailed. Good for now I would say.
| /// Lastly, it's recommended for hashers with "concat" to have reversible hashes. Refer to the | ||
| /// implementors section of [`hash::ReversibleStorageHasher`](frame_support::hash::ReversibleStorageHasher). | ||
| /// | ||
| /// ### Prefixes |
There was a problem hiding this comment.
ahh, so maybe you don't need to cover the key generation details of each storage type in each type, and tackle them all here?
There was a problem hiding this comment.
I have given an example for StorageValue and StorageMap. However, I'm still thinking about whether I should add them additionally for each type in their respective pages...
There was a problem hiding this comment.
Looks mostly good, but I have some improvement suggestions.
Next, I suggest you to tackle pallet::Config. It is probably the last "important" one to tackle among the foundations of pallets.
I think that would also be a good alignment exercise for us, so I can see how you would tackle one from scratch and give you feedback. Because I had written the foundations of this myself, it was hard for me to measure.
|
The CI pipeline was cancelled due to failure one of the required jobs. |
There was a problem hiding this comment.
LGTM, it is not perfect but HUGE step in the right direction, rather merge soon and iterate later.
It is part of paritytech/polkadot-sdk-docs#31
Yes, please go ahead. I still have quite a few ideas, but for these I'll open separate PRs later on. Curious to see how this develops :) |
|
@wentelteefje feel free to sign up to continuing with this or other macros in paritytech/polkadot-sdk-docs#31 |
This is a port (and hopefully a small improvement) of @kianenigma's PR from the old Substrate repo: paritytech/substrate#13987. Following paritytech#1689 I moved the documentation of all macros relevant to this PR from `frame_support_procedural` to `pallet_macros` while including a hint for RA users. Question: Again with respect to paritytech#1689: Is there a good reason why we should *not* enhance paths with links to our current rustdocs? For example, instead of ```rust /// **Rust-Analyzer users**: See the documentation of the Rust item in /// `frame_support::pallet_macros::storage`. ``` we could write ```rust /// **Rust-Analyzer users**: See the documentation of the Rust item in /// [`frame_support::pallet_macros::storage`](https://paritytech.github.io/polkadot-sdk/master/frame_support/pallet_macros/attr.storage.html). ``` This results in a clickable link like this: <img width="674" alt="image" src="https://github.com/paritytech/polkadot-sdk/assets/10713977/c129e622-3942-4eeb-8acf-93ee4efdc99d"> I don't really expect the links to become outdated any time soon, but I think this would be a great UX improvement over just having paths. TODOs: - [ ] Add documentation for `constant_name` macro - [x] Add proper documentation for different `QueryKinds`, i.e. `OptionQuery`, `ValueQuery`, `ResultQuery`. One example for each. Custom `OnEmpty` should be moved to `QueryKinds` trait doc page. - [ ] Rework `type_value` docs --------- Co-authored-by: kianenigma <kian@parity.io>
This is a port (and hopefully a small improvement) of @kianenigma's PR from the old Substrate repo: paritytech/substrate#13987. Following #1689 I moved the documentation of all macros relevant to this PR from
frame_support_proceduraltopallet_macroswhile including a hint for RA users.Question: Again with respect to #1689: Is there a good reason why we should not enhance paths with links to our current rustdocs? For example, instead of
we could write
This results in a clickable link like this:
I don't really expect the links to become outdated any time soon, but I think this would be a great UX improvement over just having paths.
TODOs:
constant_namemacroQueryKinds, i.e.OptionQuery,ValueQuery,ResultQuery. One example for each. CustomOnEmptyshould be moved toQueryKindstrait doc page.type_valuedocs